Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(platform): added pagination & loading state for variables page #787

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

feat(platform): added pagination & loading state for variables page #787

wants to merge 10 commits into from
+102 −23

Conversation

Allan2000-Git
Copy link
Contributor

@Allan2000-Git Allan2000-Git commented Feb 23, 2025 •
edited by codiumai-pr-agent-free bot
Loading

User description

Description

  1. Displays the first 10 variables by default. A "Load More" button loads the next 10 variables on click.
  2. Implemented hasMore flag which prevents unnecessary API calls when no more data is available.
  3. Displays a loading state while fetching variables.

Fixes #706

Dependencies

N/A

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

https://www.loom.com/share/c4547494833648858cb80c78b214c784

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

PR Type

Enhancement

Description

  • Added dynamic pagination to the variables page.

  • Implemented a "Load More" button for additional variables.

  • Introduced a loading state and error handling for fetching variables.

  • Added a skeleton loader component for improved user experience.

Changes walkthrough 📝

Relevant files
Enhancement
page.tsx
Added pagination and loading state to variables page         

apps/platform/src/app/(main)/(project)/[workspace]/[project]/@variable/page.tsx

  • Added pagination logic with page and hasMore states.
  • Implemented a "Load More" button to fetch additional variables.
  • Added error handling and loading state for API calls.
  • Updated UI to display skeleton loaders during data fetch.
    		• +83/-23 
    index.tsx
    Added skeleton loader for variables page                                 

    apps/platform/src/components/dashboard/variable/variableLoader/index.tsx

  • Created a skeleton loader component for variables.
  • Added UI elements for loading placeholders.
    		• +19/-0   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    706 - PR Code Verified

    Compliant requirements:

    • Add dynamic pagination on variables page
    • Show first 10 variables by default (ITEMS_PER_PAGE = 10)
    • Add "Load More" button to show next 10 variables on click

    Requires further human verification:

    • Verify that the UI matches the provided Figma design
    • Test pagination functionality with real data to ensure smooth loading
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The error handling could be improved by providing more specific error messages based on the error type, rather than using a generic message for all failures

    } catch (error) {
  // eslint-disable-next-line no-console -- debug error handling
  console.error('Error fetching variables:', error)
  toast.error('Failed to fetch variables', {
    description: <p className="text-xs text-red-300">
      Something went wrong while fetching variables. Please try again.
    </p>
  })
} finally {
    State Management

    The hasMore flag is only set when fewer items than ITEMS_PER_PAGE are received, but not reset when switching projects. This could lead to incorrect pagination state

    if (data.items.length < ITEMS_PER_PAGE) {
  setHasMore(false)
}

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 23, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle missing data in API response

    Add error handling for the case when the API call fails but success is true with
    no data. Currently, this edge case is not properly handled and could lead to
    undefined behavior.

    apps/platform/src/app/(main)/(project)/[workspace]/[project]/@variable/page.tsx [58-64]

     const { data, success } = await getAllVariablesOfProject()
-if (success && data) {
+if (success && data?.items) {
   setVariables((prev) => page === 0 ? data.items : [...prev, ...data.items])
   if (data.items.length < ITEMS_PER_PAGE) {
     setHasMore(false)
   }
+} else {
+  toast.error('Invalid response format', {
+    description: <p className="text-xs text-red-300">Unable to load variables. Please try again.</p>
+  })
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion addresses a critical edge case where the API returns success but with missing data, which could cause runtime errors. The improved code adds proper error handling and user feedback.

    		Medium
    Prevent memory leaks on unmount

    Add a cleanup function in useEffect to prevent state updates on unmounted
    component, which could lead to memory leaks.

    apps/platform/src/app/(main)/(project)/[workspace]/[project]/@variable/page.tsx [45-54]

     useEffect(() => {
+  let isMounted = true;
   const fetchVariables = async () => {
     if (!selectedProject) {
       toast.error('No project selected', {
         description: <p className="text-xs text-red-300">
           Please select a project to view variables.
         </p>
       })
       return
     }
+    if (!isMounted) return;
+  }
+  fetchVariables();
+  return () => {
+    isMounted = false;
+  };
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion prevents potential memory leaks by adding cleanup logic to cancel pending operations when the component unmounts, which is important for component lifecycle management and preventing state updates on unmounted components.

    		Medium
    • Update

    @rajdip-b
    Copy link
    Member

    #780

    can you refer to this? the implementation and the comments.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @kriptonian1 kriptonian1 Awaiting requested review from kriptonian1 kriptonian1 is a code owner

    At least 1 approving review is required to merge this pull request.

    Assignees
    No one assigned
    Labels
    Review effort 2/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Add pagination on variable screen
    2 participants
    @Allan2000-Git @rajdip-b